Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (b7b27aa) to head (3413abc).
⚠️ Report is 1 commits behind head on main.

@@            Coverage Diff             @@
##              main      #783    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           16        18     +2     
  Lines         1794      1968   +174     
==========================================
+ Hits          1794      1968   +174     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tomvothecoder @lee1043 @pochedls @chengzhuzhang As it was pointed out last meeting using mask as the accessor name will not work. It's currently named "geo_mask" temporarily. Any suggestions on a name or should we just moved the methods under another existing accessor?

Notes from today's meeting (07/30/25)

• Rename accessor to geomask
• Rename accessor methods sea() to mask_sea() and land() to mask_land()
• Open a separate GitHub issue to explore performance enhancements using numba

@tomvothecoder @lee1043 @pochedls @chengzhuzhang
This just occurred to me, it's currently non-trivial to pass method (regionmask or pcmdi) specific options.

Option 1:
I can change the accessor methods to be named by method and add keep argument.

ds.geomask.regionmask("pr", keep="sea", ...)

ds.geomask.pcmdi("pr", keep="land", threshold1=0.5, threshold2=0.8, source=improved_navy_land)

Option 2:
Add an options argument that will be passed to the underlying method.

ds.geomask.mask_land("pr", method="pcmdi", options={"threshold1": 0.5, "threshold2": 0.8, source: improved_navy_land})

I'd prefer a generic function where you specify the method (i.e., Option 2). I think there must be a way of avoiding passing in the arguments as a Dict. Couldn't you do something like this (or would this violate best practices in some way):

def mask_land(vid, method='pcmdi', threshold1=0.5, threshold2=0.8, source=improved_navy_land):
    """
    ...
    Parameters:
    ------------
    vid ...
    method ...
    threshold1 (float) : [...description...]. Parameter used with 'pcmdi' method (otherwise ignored).
                            Default value is 0.5.
    ...
    """
    if method == 'pcmdi':
        return _pcmdi_land_sea_mask(..., threshold1=threshold1, threshold2=threshold2, ...)
    elif method == 'regionmask'
        ...
    else:
        raise ValueError('Invalid method supplied...')

If the user supplies method1 for regionmask, it just isn't passed in (and this is noted in the docstring). If the user uses the pcmdi method and supplies [does not supply] an argument for method1 then the their value [the default value] is used.

@pochedls I could do that but I'm worried it could be a little confusing/overwhelming. Confusing in the sense of no clear indication which method the arguments work for besides being in the docstring. Overwhelming if we end up adding additional methods or arguments for the methods.

I could do it similar to how it was done in the regridder methods, where it's a ** argument that just gets passed down and the there's a reference in the documentation to see the individual method docstrings for details.

    def mask_sea(
        self,
        data_var: str,
        method: str = "regionmask",
        criteria: float | None = None,
        mask: xr.DataArray | None = None,
        output_mask: bool = False,
        **options: Any,
    ):

@pochedls I could do that but I'm worried it could be a little confusing/overwhelming. Confusing in the sense of no clear indication which method the arguments work for besides being in the docstring. Overwhelming if we end up adding additional methods or arguments for the methods.

I agree with you @jasonb5. It can be confusing and hard for users to validate which parameters apply to which method, resulting in unexpected results. Also the function signature can be too long and IDE/type hints will be less useful.

I could do it similar to how it was done in the regridder methods, where it's a ** argument that just gets passed down and the there's a reference in the documentation to see the individual method docstrings for details.

    def mask_sea(
        self,
        data_var: str,
        method: str = "regionmask",
        criteria: float | None = None,
        mask: xr.DataArray | None = None,
        output_mask: bool = False,
        **options: Any,
    ):

I learned that this is a "hybrid dispatcher pattern" that is a very common and accepted approach in scientific Python libraries (e.g., Xarray, MatPlotLib, etc.). There is one public API with multiple backends that have different implements. It keeps the main entry point simple by defining the shared parameters upfront with optional arguments passed via **options.

I think this is a reasonable approach, as long as we document it well, use strict option checking, and structure the internal dispatch clearly. We already use this pattern in the regridder too.

I am good with whatever way you choose, but I have to admit that it is inconvenient that the regridder arguments are embedded across several docstrings (that users may have trouble locating). In this issue/case, the docstring wouldn't actually be that long (see xr.open_mfdataset for an example of a pretty long docstring).

I don't find it confusing that some arguments would apply under some conditions but not others (if it is noted in the docstring).

With that said, I'm okay with docstrings being broken up, but I do have a preference for a main mask call (e.g., mask_land) versus two calls based on method (pcmdi or regionmask).

@tomvothecoder Do we have a documentation section for methods not associated with an accessor?

@tomvothecoder Do we have a documentation section for methods not associated with an accessor?

If you're referring to stand-alone functions, there is a Top-level API Functions section. We might want to split this section to sub-sections at some point, as it is growing pretty long.

@tomvothecoder The function I want to add is not top-level. It's a function in mask.py that contains the documentation for the PCMDI method options. I was going to just link the function in the accessor methods docstring.

@tomvothecoder The function I want to add is not top-level. It's a function in mask.py that contains the documentation for the PCMDI method options. I was going to just link the function in the accessor methods docstring.

I see. If you want to list that function in the docs, we can create a separate section for module-level functions.

@tomvothecoder @lee1043 This can be tested now. I updated the documentation to include the module-level methods.

@jasonb5 Can you provide minimum reproducible examples with the two methods, ideally with some test data? This will help expedite the code review process.

@jasonb5 Can you provide minimum reproducible examples with the two methods, ideally with some test data? This will help expedite the code review process.

Actually, I can probably just create a script using the xcdat-data repo. Feel free to provide one if you have it though.

@jasonb5 @lee1043 The navy_land.nc file is 8.96mb, which bloats the repo. We should host this file in xcdat-data and use pooch to download this file with caching if the user calls the pcmdi masking functionality. I don't think that many people will be using this masking method, considering it is specialized and regionmask is the default.

@jasonb5 @lee1043 The navy_land.nc file is 8.96mb, which bloats the repo. We should host this file in xcdat-data and use pooch to download this file with caching if the user calls the pcmdi masking functionality. I don't think that many people will be using this masking method, considering it is specialized and regionmask is the default.

@tomvothecoder Agreed, hosting the data somewhere and download only when it is needed sounds a good solution to me.

@jasonb5 @lee1043 The navy_land.nc file is 8.96mb, which bloats the repo. We should host this file in xcdat-data and use pooch to download this file with caching if the user calls the pcmdi masking functionality. I don't think that many people will be using this masking method, considering it is specialized and regionmask is the default.

I pushed 04cc38d (#783) to use pooch for fetching and caching navy_land.nc from xcdat-data (here) if the PCMDI land sea mask functionality is used. This makes the code cleaner and the bundle size is kept small.

Excellent work @jasonb5, thank you! Also thanks @lee1043 and @pochedls for the review.

@jasonb5 thank you so much for the work! And thanks to @tomvothecoder and @pochedls as well. Kudos for the team!!

@tomvothecoder @pochedls @lee1043 Thanks all, glad we got this done! Cheers!